fix: Reduce React re-renders when checking currentCard()
#3398
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- `currentCard()` is called multiple times which triggers the expensive `upcomingCardIds()` - Adding a static `currentCard` variable reduced calls (and thus multiple React re-renders)
|
Removed vultr server and associated DNS entries |
DafyddLlyr
force-pushed
the
dp/current-card-multiple-renders-bug
branch
from
83c2eaa to
e27818c
Compare
DafyddLlyr
force-pushed
the
dp/current-card-multiple-renders-bug
branch
from
e27818c to
c4f9bd2
Compare
DafyddLlyr
commented
Jul 10, 2024
| @@ -48,7 +48,7 @@ const Card: React.FC<Props> = ({ | |||
| ...props | |||
| }) => { | |||
| const theme = useTheme(); | |||
| const [path, currentCard] = useStore((state) => [ | |||
| const [path, visibleNode] = useStore((state) => [ | |||
There was a problem hiding this comment.
Throughout this PR I've tired to maintain existing variable names when deconstructing state from the store. I think this minimises the number of changes, and hopefully maintains context within the component. For example, sometimes the currentCard is referred to as the node, visibleNode or currentCard across differing components.
DafyddLlyr
commented
Jul 10, 2024
| @@ -379,6 +380,7 @@ export const previewStore: StateCreator< | |||
|
|
|||
| resumeSession(session: Session) { | |||
| set({ ...session }); | |||
| get().setCurrentCard(); | |||
There was a problem hiding this comment.
When resuming, we handle the initial state setup here after getting a ReconciliationResponse
Please see #1701 for context.
jessicamcinchak
approved these changes
Jul 11, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What's the problem?
What's the cause?
When running the application in dev mode, the following two errors generally coincide with the above occurrence -
The stack traces of these errors point to the following lines of code respectively -
planx-new/editor.planx.uk/src/pages/FlowEditor/lib/store/preview.ts
Line 512 in 20ce9d6
https://github.com/theopensystemslab/planx-new/blob/main/editor.planx.uk/src/@planx/components/Section/Public.tsx#L31
After a lot of trial and error debugging these sections it essentially appears that the problem is caused by
upcomingCardIds()(called when hitting "back") clashing with the rendering of theAnalyticsProvider(which passed the updatednodedown to it's descendants).What's the solution?
currentCard()upcomingCards()and is ultimately responsible for displaying a card to the userrecord())currentCard()is called as a shorthand to get the current node Id, so I've made a new property in the store to hold this variableupcomingCardIds(), just referring to the storedcurrentCard()from the last call.nodefrom theAnalyticsProvider, and just store in Zustand